test: Make the ReadRows service in tests more modular #1462
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Replace with as ServerImplementationInterface * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Fix a regression bug from the merge --------- Co-authored-by: Leah E. Cole <[email protected]>
…sion of the first ReadRows (#1457) * Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Set up the second ReadRows service to use params * Remove duplicate copy of generate chunks from serv * Remove second copy of isKeyInRowSet * Eliminate duplicate copies of the debug log * Fix a bug for the to and from service parameters Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy. * Add cancel back into the mock service for testing * Add variables to match service 1 * Add one more check to match the other service * Remove usages of ReadRows2Impl * Remove the new service The old service has been generalized enough to mock correct server behaviour. * Moved the position of the comment for 150 rows * Eliminate setting keyTo and keyFrom they are undefined anyway. * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Delete file * Change splitted to split * Remove export * Don’t rename the interfaces There is no point * Increase Windows timeout * Provide functions to eliminate if statements * Eliminate the if statements Make a direct call to generateChunksFromRequest * Revert "Eliminate the if statements" This reverts commit 0996e89. * Revert "Provide functions to eliminate if statements" This reverts commit 4a4761f. * Change any’s to string | undefined * Eliminate duplicate code for setting timeouts * remove only * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> * Update test/readrows.ts Co-authored-by: Leah E. Cole <[email protected]> --------- Co-authored-by: Leah E. Cole <[email protected]>
…educing the size of it significantly (#1460) * Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Set up the second ReadRows service to use params * Remove duplicate copy of generate chunks from serv * Remove second copy of isKeyInRowSet * Eliminate duplicate copies of the debug log * Fix a bug for the to and from service parameters Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy. * Add cancel back into the mock service for testing * Add variables to match service 1 * Add one more check to match the other service * Remove usages of ReadRows2Impl * Remove the new service The old service has been generalized enough to mock correct server behaviour. * Moved the position of the comment for 150 rows * Eliminate setting keyTo and keyFrom they are undefined anyway. * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Group the property getter into a function * Group key selection into functions * Fix typo: default key * Documentation for isInRowSet * Eliminate variable - move function inline * Omit optional selector on stream * Create ReadRowsWritableStream interface * Use new interface, remove ServerWritableStream * Don’t pass the whole stream into helpers * Add a function for generating the chunks * The debug log should be a pass through parameter * Solve compiler errors resulting immediately from merge * Add debugLog parameter to various functions * Add return type * Remove exports - functions are only used in this module * Revise merge correction * Remove TODO for the stream type * Update the getKeyValue function * Eliminate place where debug log is needed * Run linter
…e long function (#1461) * Use prettyPrintRequest from readRowsImpl1 * Define interfaces for creating a Bigtable service * Remove keyFrom and keyTo from each test * Define the service inline for standard keys errors * Add a comment about the refactor steps I am doing * Add a header to the service parameters file * Use the ChunkGeneratorParameters interface * Simplify the diff by creating local variables * Remove comment * Eliminate chunks per response constant * Change imports * Set up the second ReadRows service to use params * Remove duplicate copy of generate chunks from serv * Remove second copy of isKeyInRowSet * Eliminate duplicate copies of the debug log * Fix a bug for the to and from service parameters Only ignore keyFrom and keyTo when they are undefined and not just when they are Falsy. * Add cancel back into the mock service for testing * Add variables to match service 1 * Add one more check to match the other service * Remove usages of ReadRows2Impl * Remove the new service The old service has been generalized enough to mock correct server behaviour. * Moved the position of the comment for 150 rows * Eliminate setting keyTo and keyFrom they are undefined anyway. * Add a second test for an error at a random pos * Add some comments for documentation * Chunk generation - add the parameter * Added comments to all the interfaces * Group the property getter into a function * Group key selection into functions * Fix typo: default key * Documentation for isInRowSet * Eliminate variable - move function inline * Omit optional selector on stream * Create ReadRowsWritableStream interface * Use new interface, remove ServerWritableStream * Don’t pass the whole stream into helpers * Add a function for generating the chunks * The debug log should be a pass through parameter * Add some TODOs about how to address this next * Pull send response into a class * Create a dedicated class for defining the service * Change name to readRowsRequestHandler * Pull the function that generates the chunks into Separate module * Generate documentation * Add more documentation to the Service class * Add debugLog as a method parameter * Solve compiler errors resulting immediately from merge * Add debugLog parameter to various functions * Add return type * Remove exports - functions are only used in this module * Revise merge correction * Remove TODO for the stream type * Update the getKeyValue function * Eliminate place where debug log is needed * Run linter * Eliminate the function that creates a service Add a factory method instead * Add documentation for the constructor * Remove the TODO * Set the timeout for the test * Increase the timeout
product-auto-label
bot
added
size: l
danieljbruce
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
sofisl
reviewed
Aug 19, 2024
test/utils/readRowsImpl.ts
Outdated
| ? defaultKey | ||
| : keyRequestClosed | ||
| ? parseInt(keyRequestClosed as string) | ||
| : parseInt(keyRequestOpen as string) + 1; |
There was a problem hiding this comment.
These combined ternary expressions are hard to read. Is this saying if the defaultKey is not undefined, then use default key, else use keyRequestClosed, and if that is undefined use it as opened, or else as closed? I think this might achieve the same thing and be easier to read, or if not maybe you can rewrite it in a way that preserves the logic:
return defaultKey || keyRequestClosed ? parseInt(keyRequestClosed as string) : parseInt(keyRequestOpen as string) + 1;
There was a problem hiding this comment.
We can't do it quite like this because when defaultKey is 0 it is falsy so it doesn't get used, but we want it to get used when it is 0. What I did instead was did an equal check instead of a not equal check.
| const stringKey = binaryKey.toString(); | ||
| debugLog(`sent lastScannedRowKey = ${stringKey}`); | ||
| } | ||
| if (!canSendMore) { |
There was a problem hiding this comment.
what's the difference between lastScannedRowKey and canSendMore? Maybe a comment describing the branches would be useful. Also, is it possible to have a lastScannedRowKey and !canSendMore? Otherwise perhaps an else might be useful.
There was a problem hiding this comment.
From this, canSendMore is the value returned indicating whether or not we want the stream to drain. lastScannedRowKey is actually part of the response so I think both the if statements are independent.
There was a problem hiding this comment.
This code is pretty much an exact copy of what we have on main:
nodejs-bigtable/test/utils/readRowsImpl.ts
Lines 191 to 210 in ed942c5
There was a problem hiding this comment.
I added comments for the if blocks
…nodejs-bigtable into 3527322442-to-main-2
danielbankhead
approved these changes
Aug 29, 2024
cindy-peng
approved these changes
Sep 23, 2024
bhshkh
approved these changes
Sep 23, 2024
kevkim-codes
added a commit
to kevkim-codes/nodejs-bigtable
that referenced
this pull request
Sep 27, 2024
test: Make the ReadRows service in tests more modular (googleapis#1462)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR makes the ReadRows service in tests more modular.
Background
This PR is made up of 4 already reviewed PRs merged onto this feature branch that are now being merged onto main.